Preload spec in serving cache #152
Conversation
|
/assign @tims |
|
Can you summarise what your PR in words, so it's easier for me to review? |
|
Updated the PR description |
| @@ -177,4 +209,16 @@ public StorageSpec load(String key) throws Exception { | |||
| public boolean isConnected() { | |||
| return coreService.isConnected(); | |||
| } | |||
|
|
|||
| /** Preload all spec into cache. */ | |||
| public void populateCache() { | |||
There was a problem hiding this comment.
With a MAX_SPEC_COUNT of 1000, once you have more than that, you're going to be evicting things that might actually be commonly used every 5 minutes. Is that a problem?
There was a problem hiding this comment.
Yeah, that's probably introduce problem. Do you think it's better to not limit the cache size based on the number of entry?
| } | ||
|
|
||
| @Test | ||
| public void testPopulateCache() { |
There was a problem hiding this comment.
can you add a test for what happens when you exceed the max size of cache in populating. So we're explicit about the intended behaviour.
| @@ -58,14 +59,25 @@ public EntitySpec load(String key) throws Exception { | |||
| } | |||
|
|
|||
| @Override | |||
| public Map<String, EntitySpec> loadAll(Iterable<? extends String> keys) throws Exception { | |||
| return coreService.getEntitySpecs((Iterable<String>) keys); | |||
| public ListenableFuture<EntitySpec> reload(String key, EntitySpec oldValue) | |||
There was a problem hiding this comment.
I think these can be simplified to:
entitySpecLoader = CacheLoader.asyncReloading(
CacheLoader.from(
(String key) -> coreService.getEntitySpecs(Collections.singletonList(key)).get(key)),
executorService);
featureSpecLoader = CacheLoader.asyncReloading(
CacheLoader.from(
(String key) -> coreService.getFeatureSpecs(Collections.singletonList(key)).get(key)),
executorService);
etc..
There was a problem hiding this comment.
The asyncReloading won't return old value if anything goes wrong during refresh, that's why I used a more verbose implementation.
|
/approve |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: tims The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
|
/lgtm |
* Preload spec in serving cache * Disable async refresh and only periodically refresh cache
This PR change 2 behaviour in serving component:
Fix #151